Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EEM] Replace hashed ID with human readable ID #193652

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

miltonhultgren
Copy link
Contributor

@miltonhultgren miltonhultgren commented Sep 21, 2024

This PR turns the entity.id field format from a hashed value to a human readable string of the values found in the identity fields, such as my_host-my_cloud_zone for the identity fields [host.name, cloud.availability_zone].
The order of the values is based on the order in the identity fields list.

@miltonhultgren miltonhultgren added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:EEM Elastic Entity Model labels Sep 21, 2024
@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@miltonhultgren
Copy link
Contributor Author

/ci

@miltonhultgren miltonhultgren marked this pull request as ready for review September 30, 2024 14:48
@miltonhultgren miltonhultgren requested a review from a team as a code owner September 30, 2024 14:48
Comment on lines 145 to 147
value: definition.identityFields
.map((identityField) => {
const field = `entity.identity.${identityField.field}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we sort the identityFields to make sure it is consistent even if the order of identityFields for whatever reason changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the pr desc mentions this but looks like it's not implemented yet

Copy link
Contributor Author

@miltonhultgren miltonhultgren Oct 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one problem I see with this is that any consumer of the definition must then also remember to sort the fields first before recreating the ID.

While if we rely on the order of definition, it is up to the author to ensure the order makes sense as the set of fields changes.

Could it be that it's a valid case to change the order of the fields?
It should not have any semantic meaning, but may lead to "prettier" IDs being generated depending on the values found in the used fields.

In a world where we don't do materialization (by default), this seems like a smaller problem?

We did have some discussion recently where we said that essentially, if you change the identity fields, we should just throw away any previously materialized data as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can expose utility methods that convert the id into an entity, and given that, I would personally prefer sorting the identity fields because we can sort it always when we deserialize, instead of hoping no one changed the order of the identity fields (for whatever reason).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a world where interactions go through an entities client, that should be fine, I'll add the sorting then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
inventory 152.2KB 152.3KB +73.0B

History

  • 💔 Build #236153 failed 3567c9bce6bd0149caf14f535c937d78760931f9

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@miltonhultgren miltonhultgren requested a review from a team as a code owner October 14, 2024 15:16
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Oct 14, 2024
@miltonhultgren miltonhultgren requested review from a team as code owners October 14, 2024 19:30
@@ -145,7 +145,7 @@ export type MetadataField = z.infer<typeof metadataSchema>;
export const identityFieldsSchema = z
.object({
field: z.string(),
optional: z.boolean(),
optional: z.literal(false),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This blocks people from using optional fields even though technically this is still supported in the code, and we transform the existing saved objects to remove them.

This is mainly due to not being able to change the saved object mapping.

EntityDefinition,
EntityDefinition
> = (savedObject) => {
// Doing only this may break displayNameTemplates
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which I think is okay since we're likely to remove this anyway in the future since it's hard to do any kind of template in ES|QL at runtime. So the displayNameTemplate might become similar to the identity field, where we just let users decide which fields to concat instead.

@miltonhultgren miltonhultgren requested review from a team and removed request for a team and jaredburgettelastic October 18, 2024 07:47
@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 18, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: ada614c
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-193652-ada614c40f53

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
inventory 218.6KB 218.6KB +2.0B

History

Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsafe transform LGTM.

@miltonhultgren miltonhultgren merged commit ae2c6ad into elastic:main Oct 18, 2024
25 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11405848322

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 18, 2024
This PR turns the `entity.id` field format from a hashed value to a
human readable string of the **values** found in the identity fields,
such as `my_host-my_cloud_zone` for the identity fields `[host.name,
cloud.availability_zone]`.
The order of the values is based on the order in the identity fields
list.

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit ae2c6ad)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 18, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [[EEM] Replace hashed ID with human readable ID
(#193652)](#193652)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Milton
Hultgren","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-18T14:42:38Z","message":"[EEM]
Replace hashed ID with human readable ID (#193652)\n\nThis PR turns the
`entity.id` field format from a hashed value to a\r\nhuman readable
string of the **values** found in the identity fields,\r\nsuch as
`my_host-my_cloud_zone` for the identity fields
`[host.name,\r\ncloud.availability_zone]`.\r\nThe order of the values is
based on the order in the identity
fields\r\nlist.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"ae2c6ad321f2b4318d4114c1309b4420861bcd29","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Feature:EEM"],"title":"[EEM]
Replace hashed ID with human readable
ID","number":193652,"url":"https://github.com/elastic/kibana/pull/193652","mergeCommit":{"message":"[EEM]
Replace hashed ID with human readable ID (#193652)\n\nThis PR turns the
`entity.id` field format from a hashed value to a\r\nhuman readable
string of the **values** found in the identity fields,\r\nsuch as
`my_host-my_cloud_zone` for the identity fields
`[host.name,\r\ncloud.availability_zone]`.\r\nThe order of the values is
based on the order in the identity
fields\r\nlist.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"ae2c6ad321f2b4318d4114c1309b4420861bcd29"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193652","number":193652,"mergeCommit":{"message":"[EEM]
Replace hashed ID with human readable ID (#193652)\n\nThis PR turns the
`entity.id` field format from a hashed value to a\r\nhuman readable
string of the **values** found in the identity fields,\r\nsuch as
`my_host-my_cloud_zone` for the identity fields
`[host.name,\r\ncloud.availability_zone]`.\r\nThe order of the values is
based on the order in the identity
fields\r\nlist.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"ae2c6ad321f2b4318d4114c1309b4420861bcd29"}}]}]
BACKPORT-->

Co-authored-by: Milton Hultgren <[email protected]>
tiansivive pushed a commit to tiansivive/kibana that referenced this pull request Oct 31, 2024
This PR turns the `entity.id` field format from a hashed value to a
human readable string of the **values** found in the identity fields,
such as `my_host-my_cloud_zone` for the identity fields `[host.name,
cloud.availability_zone]`.
The order of the values is based on the order in the identity fields
list.

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit ae2c6ad)
tiansivive added a commit that referenced this pull request Oct 31, 2024
)

# Backport

This will backport the following commits from `main` to `8.16`:
 - [EEM] Replace hashed ID with human readable ID (#193652) (ae2c6ad)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Milton
Hultgren","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-18T14:42:38Z","message":"[EEM]
Replace hashed ID with human readable ID (#193652)\n\nThis PR turns the
`entity.id` field format from a hashed value to a\r\nhuman readable
string of the **values** found in the identity fields,\r\nsuch as
`my_host-my_cloud_zone` for the identity fields
`[host.name,\r\ncloud.availability_zone]`.\r\nThe order of the values is
based on the order in the identity
fields\r\nlist.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"ae2c6ad321f2b4318d4114c1309b4420861bcd29"},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[]}]
BACKPORT-->

Co-authored-by: Milton Hultgren <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) ci:project-deploy-observability Create an Observability project Feature:EEM Elastic Entity Model release_note:skip Skip the PR/issue when compiling release notes v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants